Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove shorts from Subscriptions tab #201

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Remove shorts from Subscriptions tab #201

merged 1 commit into from
Oct 18, 2024

Conversation

JaCzekanski
Copy link

This PR adds filtering of JSON nodes containing reelWatchEndpoint which is an indicator that given video is a reel/short.
Due to the convoluted format of JSON responses, it might not cover all of the cases.

Reel playback seems broken on TVs and I don't really care about having them, as it's ofter reused content.
Please note that it'll only work on the Subscriptions tab (which is the one I use the most).
However, It should be pretty easy to extend it to other tabs and search results.

Before:
Screenshot 2024-09-18 at 13 45 03

After:
Screenshot 2024-09-18 at 13 45 21

Green settings menu:
Screenshot 2024-09-18 at 13 45 37

@JaCzekanski JaCzekanski mentioned this pull request Sep 18, 2024
Open
@FuriousBottle
Copy link

Great work, keen to see this merged - shorts are a real pain point for me

@JaCzekanski
Copy link
Author

@FuriousBottle Is there any action needed from my side to have this PR merged?

@FuriousBottle
Copy link

@JaCzekanski sorry for the confusion, I am just an admirer, I don't have any authority here 😅

throwaway96
throwaway96 requested changes Oct 7, 2024
View reviewed changes
Copy link
Member

@throwaway96 throwaway96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. I'll merge it after these things are addressed.

package-lock.json Outdated Show resolved Hide resolved
src/ui.js Show resolved Hide resolved
src/shorts.js Outdated Show resolved Hide resolved
@JaCzekanski
Copy link
Author

I tested the current "hardcoded path" approach and recursive search - the latter is slightly slower (~0.2ms on nodejs, mac), but we don't have to store the path so it should be more maintainable in the future.

https://gist.github.com/JaCzekanski/7a35e15473d5bbe62056e6e03e3f5d17

$ node shorts-test.js  # initial Subscriptions response (gridRenderer)
original JSON.parse: 1.969ms
hardcoded remove shorts: 1.842ms
find remove shorts: 2.126ms

$ node shorts-test.js  # continuation response (gridContinuation)
original JSON.parse: 0.399ms
hardcoded remove shorts: 0.397ms
find remove shorts: 0.454ms

throwaway96
throwaway96 approved these changes Oct 18, 2024
View reviewed changes
Copy link
Member

@throwaway96 throwaway96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I would like to test it before merging though. I will try to make some time for YTAF this weekend.

@throwaway96
Copy link
Member

I'm not really able to properly test the functionality, but at least it doesn't seem to break anything on K7Lp.

@throwaway96 throwaway96 merged commit 0fc3aff into webosbrew:main Oct 18, 2024
1 check passed
@teolabx
Copy link

teolabx commented Oct 20, 2024

Hello guys, please tell me when there will be an update with the function of hiding shorts, or now I can replace .js files in the file system via webos dev manager. But I don't know the way. Thanks for the information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fire332 fire332 fire332 left review comments

@throwaway96 throwaway96 throwaway96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JaCzekanski @FuriousBottle @throwaway96 @teolabx @fire332